tests/test_directory.py (1)

144-162: Strengthen the group-filter test so it actually validates filtering/wiring.

At Lines 157-161, asserting len(response[0].users) == 0 with a nonexistent group can pass even if directory_group_id is ignored (because the directory may already have no users). Consider asserting request wiring via a unit-level mock or using fixture data where unfiltered results are non-empty and filtered results differ.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_directory.py` around lines 144 - 162, The test
test_list_directory_users_filter_by_group currently asserts zero users for a
nonexistent directory_group_id which can pass even if directory_group_id is
ignored; change it to explicitly validate the filter wiring by either (a)
creating fixture data: create at least one user in the directory and a separate
user assigned to a real group, then call list_directory_users twice (once
without directory_group_id and once with a valid directory_group_id) and assert
the unfiltered response contains users while the filtered response only contains
the group member(s), or (b) use a unit-level mock for
scalekit_client.directory.list_directory_users to assert it was called with the
directory_group_id parameter; locate the test by function name
test_list_directory_users_filter_by_group and update calls to
list_directory_users and the setup using CreateDirectory/CreateOrganization to
ensure the scenario proves the filter is applied.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/test_directory.py (3)

159-159: 💤 Low value

Direct os.environ[...] access yields an opaque KeyError when unset.

If SCALEKIT_ENV_URL isn't configured, this fails with a bare KeyError rather than a clear skip/assertion. Consider guarding it.

♻️ Optional guard

-        env_url = os.environ['SCALEKIT_ENV_URL'].rstrip('/')
+        env_url = os.environ.get('SCALEKIT_ENV_URL')
+        if not env_url:
+            self.skipTest("SCALEKIT_ENV_URL not configured")
+        env_url = env_url.rstrip('/')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_directory.py` at line 159, The test currently reads
SCALEKIT_ENV_URL directly into env_url which raises a bare KeyError if the env
var is missing; change the access in tests/test_directory.py so you first
retrieve the value with os.environ.get or os.getenv('SCALEKIT_ENV_URL') and, if
it's None/empty, call pytest.skip (or assert with a clear message) before using
.rstrip('/'); update the env_url assignment and any subsequent use to handle the
guarded case and reference SCALEKIT_ENV_URL and the env_url variable so the test
fails/skips with a clear message instead of raising KeyError.

---

173-189: ⚡ Quick win

Add a timeout to the SCIM requests.post calls.

Both POSTs lack a timeout, so a stalled SCIM endpoint can hang the test run indefinitely. Flagged by static analysis (S113).

♻️ Add timeouts

         user_r = requests.post(f'{scim_base}/Users', json={
             'schemas': ['urn:ietf:params:scim:schemas:core:2.0:User'],
             'userName': email,
             'name': {'givenName': fake.first_name(), 'familyName': fake.last_name()},
             'emails': [{'value': email, 'primary': True}],
             'active': True
-        }, headers=headers)
+        }, headers=headers, timeout=30)

         group_r = requests.post(f'{scim_base}/Groups', json={
             'schemas': ['urn:ietf:params:scim:schemas:core:2.0:Group'],
             'displayName': fake.bs(),
             'members': [{'value': scim_user_id}]
-        }, headers=headers)
+        }, headers=headers, timeout=30)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_directory.py` around lines 173 - 189, The two SCIM POST calls
creating user_r and group_r currently call requests.post without a timeout;
update both calls (the requests.post that assigns user_r and the requests.post
that assigns group_r) to pass an explicit timeout (e.g.
timeout=<reasonable_seconds> or a TEST_TIMEOUT constant) so the tests fail fast
on a stalled SCIM endpoint; ensure both requests include the same timeout value
and run the tests to confirm no breakage.

---

199-200: 🏗️ Heavy lift

Remove the SCIM-id vs directory-id mismatch concern; keep an eventual-consistency/flakiness guard

• list_directory_users populates response[0].users[].id by copying the underlying ListDirectoryUsers user.id directly (dir_user.id = user.id), and the protobuf documents DirectoryUser.id as Scalekit’s “User ID” (diruser_*). Since the test uses the SCIM id both for group members[].value and for the final users[0].id assertion, the identifier types are aligned with what the directory API returns.
• The only remaining risk is timing: if the SCIM POSTs aren’t immediately reflected in list_directory_users, lines 199-200 may become flaky; add polling/retry (or an explicit sync trigger) if failures occur.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_directory.py` around lines 199 - 200, The test's assertion
comparing response[0].users[0].id to scim_user_id is valid (IDs align), but may
be flaky due to eventual consistency; update the test around
list_directory_users to retry/poll until the expected user appears (e.g., call
list_directory_users in a short loop with sleeps and a timeout) instead of a
single immediate assert, so functions/methods involved are list_directory_users
and the test's scim POST flow that creates the user; keep the same final
equality assertion but gate it behind the retry/poll loop to avoid transient
failures.